samples: siwx917_ota: http/s OTAF application #111
Conversation
Devika-Raju-Z
force-pushed
the
https_otaf
branch
from
466273e to
6a581ad
Compare
Devika-Raju-Z
force-pushed
the
https_otaf
branch
24 times, most recently
from
569c7b4 to
dd7f3de
Compare
jhedberg
changed the title
|
Please rebase this on the latest |
Devika-Raju-Z
force-pushed
the
https_otaf
branch
2 times, most recently
from
ee3fcc1 to
ec8d3a0
Compare
jerome-pouiller
left a comment
jerome-pouiller
left a comment
There was a problem hiding this comment.
I have not yet finish my review, but the things start to have a good shape.
jerome-pouiller
left a comment
jerome-pouiller
left a comment
There was a problem hiding this comment.
I have finished the review.
Devika-Raju-Z
force-pushed
the
https_otaf
branch
3 times, most recently
from
5b70aa4 to
f944c8b
Compare
jerome-pouiller
left a comment
jerome-pouiller
left a comment
There was a problem hiding this comment.
We are mostly fine.
When you send new version of your PR, keep the same parent commit (avoid rebase on the last main). Otherwise, the reviewer cannot diff between the previous version and the new one.
samples/siwx91x_ota/src/main.c
Outdated
| case OTA_STATE_SCAN: | ||
| /* Scan for networks and connect */ | ||
| ota_wifi_start_scan(ctx); | ||
| ret = ota_wifi_connect(ssid, pwd, ctx); |
There was a problem hiding this comment.
I didn't get it until now, but isn't there a race condition between scan, connect and security_type field?
Devika-Raju-Z
force-pushed
the
https_otaf
branch
from
f944c8b to
7807865
Compare
samples/siwx91x_ota/src/main.c
Outdated
| static int ota_download_firmware_chunk(struct app_ctx *ctx) | ||
| { | ||
| char range_header[64]; | ||
| const char *headers[] = { range_header, NULL }; |
There was a problem hiding this comment.
The compliance Ci want to see this array declared with static (or you can remove the const I assume)
There was a problem hiding this comment.
The CI still complain. Maybe char *headers[] would be fine.
Devika-Raju-Z
force-pushed
the
https_otaf
branch
6 times, most recently
from
fa56326 to
abb9387
Compare
This application demonstrates the support for OTA firmware upgrade. Signed-off-by: Devika Raju <Devika.Raju@silabs.com>
samples/siwx91x_ota/src/main.c
Outdated
| } | ||
| } | ||
| printf("Maximum retries exceeded, aborting OTA\n"); | ||
| return -EIO; |
There was a problem hiding this comment.
Just so there's no misconception, returning an error here will not make any difference since Zephyr ignores it: https://github.com/zephyrproject-rtos/zephyr/blob/53eaf0f71ee9d401b974006c926811f6a3fc4701/kernel/init.c#L347
I.e. the effect is simply that the thread exits, which is probably fine for the purposes of this app.
samples/siwx91x_ota/src/main.c
Outdated
| uint8_t http_recv_buffer[1024]; | ||
| }; | ||
|
|
||
| static char *ota_get_url_field(char *url, struct http_parser_url parser, int field) |
There was a problem hiding this comment.
Should be const char *url, I believe
samples/siwx91x_ota/src/main.c
Outdated
| static char *ota_get_url_field(char *url, struct http_parser_url parser, int field) | ||
| { | ||
| int len = parser.field_data[field].len; | ||
| char *ptr = url + parser.field_data[field].off; |
| char *out; | ||
|
|
||
| __ASSERT(parser.field_set & BIT(field), "Invalid URL"); | ||
| out = malloc(len + 1); |
There was a problem hiding this comment.
Is there a reason you went with the dynamic memory allocation option instead of copying the entire URL to a modifiable buffer, and then adding \0 characters into it at the appropriate offsets whenever you need to access fields as a nul terminated C string? That's what other code in the Zephyr tree seems to be doing (e.g. lwm2m).
There was a problem hiding this comment.
The problem is for the UF_PATH. Typically, in http://foo.com/file.rps, if you replace / with \0, it overlaps /file.rps.
There was a problem hiding this comment.
(BTW, I see the code still have the copy to the modifiable buffer because I made the same suggestion earlier. Probably we should now remove this copy.)
There was a problem hiding this comment.
Right, that can indeed be problematic. There is a solution though (although does require some extra code): if you don't need to access the fields simultaneously, store the overwritten character in a temporary variable, and then put it back when you're done with the C string-style access. This is also what lwm2m is doing: https://github.com/zephyrproject-rtos/zephyr/blob/53eaf0f71ee9d401b974006c926811f6a3fc4701/subsys/net/lib/lwm2m/lwm2m_message_handling.c#L3382-L3383
There was a problem hiding this comment.
Just so we're clear: this isn't a "hard block" from me, rather I'd just like to know what options were considered, since dynamic memory allocation is usually undesirable in Zephyr code.
Devika-Raju-Z
force-pushed
the
https_otaf
branch
from
abb9387 to
bdf6d6a
Compare
4 participants
Updating the SiWx917 device's firmware including NWP, M4, and the combined M4 + TA images using an HTTP or HTTPS server. It supports both secure and non-secure firmware updates for all three types: NWP, M4, and the combined image.
Note
Adds a new Zephyr sample for SiWx917 that performs HTTP/HTTPS OTA firmware updates over Wi‑Fi with ranged downloads and firmware flashing.
samples/siwx91x_otasrc/main.c: Implements Wi‑Fi connect/scan, IP config handling, DNS logging, and an OTA state machine (scan → server connect → ranged download) usinghttp_clientwith TLS and Range headers; buffers chunks, validates image size, loads viasl_si91x_fwup_*, and reboots on success.prj.conf: Enables networking, sockets, DHCPv4, HTTP client, URL parser, mbedTLS (TLS 1.2), DNS resolver, and SiWx91x firmware upgrade support.Kconfig: Adds configurableCONFIG_OTA_WIFI_SSID,CONFIG_OTA_WIFI_PSK, andCONFIG_OTA_UPDATE_URL.CMakeLists.txt: Sets up app build and sources.README.rst: Documents purpose, configuration, build/run, and test steps.sample.yaml: Registers sample and net test forsiwx917_rb4338a.Written by Cursor Bugbot for commit 8c6da5c. This will update automatically on new commits. Configure here.